-
Notifications
You must be signed in to change notification settings - Fork 39
Dynamic updating of event interface on event admin change. #425
base: master
Are you sure you want to change the base?
Dynamic updating of event interface on event admin change. #425
Conversation
@@ -120,25 +120,34 @@ $(document).ready(function() { | |||
var firstScriptTag = document.getElementsByTagName('script')[0]; | |||
firstScriptTag.parentNode.insertBefore(tag, firstScriptTag); | |||
|
|||
this.currentUser = new models.User(USER); | |||
this.currentUserIsAdmin = function() { | |||
return this.currentUser.isAdminOf(curEvent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The this
value here isn't enclosed -- won't this.currentUser
be undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because this.currentUserIsAdmin() is only called in the context of the initializer. I've tested it, it works. :) Did you have some other approach in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you that it works. I'm concerned about the code style of going 3 levels deep in the call stack with this
and having it work -- it seems brittle; any variation in how those methods get invoked and the binding is lost.
We could either explicitly bind "this" in the function definitions (e.g. (function() {}).bind(this)
), or enclose a reference to this which isn't dynamic, e.g.:
var that = this
that.currentUserIsAdmin = function() {
return that.currentUser.isAdminOf(curEvent);
};
Just concerned about maintainability of that code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've addressed your concern by binding 'this' to the function.
I'd love to have a small test of this, to ensure we don't accidentally break it. The event-session-messages test is probably a good template or this: set up an event, point selenium at it, and log a regular non-admin user at it. Then, via a socket message, change the user to admin status, and observe the change in UI for the now-admin. |
This patch refactors all event rendering functionality to enable updating the event interface in the case where an event admin is added or removed. Only the user who was added or removed has their event interface updated.
This patch refactors all event rendering functionality to enable
updating the event interface in the case where an event admin is
added or removed. Only the user who was added or removed has their
event interface updated.
We needed this for fast switching of admins in our installation.
This patch depends on #424.